-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Profile: allocate buffer for n instruction pointers per thread #41821
Profile: allocate buffer for n instruction pointers per thread #41821
Conversation
@@ -40,7 +40,7 @@ end | |||
init(; n::Integer, delay::Real)) | |||
|
|||
Configure the `delay` between backtraces (measured in seconds), and the number `n` of | |||
instruction pointers that may be stored. Each instruction pointer corresponds to a single | |||
instruction pointers that may be stored per thread. Each instruction pointer corresponds to a single |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a !!! compat
note?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to a compat note
@@ -40,7 +40,7 @@ end | |||
init(; n::Integer, delay::Real)) | |||
|
|||
Configure the `delay` between backtraces (measured in seconds), and the number `n` of | |||
instruction pointers that may be stored. Each instruction pointer corresponds to a single | |||
instruction pointers that may be stored per thread. Each instruction pointer corresponds to a single |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to a compat note
8080207
to
afca441
Compare
It looks like this broke the linux32 build, as seen in the pre-merge CI |
Sorry about that. The only way I can think that this PR is causing the error below is if somehow the
|
It is attempting to initialize profile space for 128 threads, and now failing (OOM) |
128 real threads, or is that a bug also? If that's real, then options seem:
I guess it's not reliable that a n-thread machine would have n-times more ram than a 1-thread machine, which this PR kind of assumes |
""" | ||
function init(; n::Union{Nothing,Integer} = nothing, delay::Union{Nothing,Real} = nothing) | ||
n_cur = ccall(:jl_profile_maxlen_data, Csize_t, ()) | ||
delay_cur = ccall(:jl_profile_delay_nsec, UInt64, ())/10^9 | ||
if n === nothing && delay === nothing | ||
return Int(n_cur), delay_cur | ||
end | ||
nnew = (n === nothing) ? n_cur : n | ||
nthreads = Sys.iswindows() ? 1 : Threads.nthreads() # windows only profiles the main thread | ||
nnew = (n === nothing) ? n_cur : n * nthreads |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's also a bug here. This method passes to the method below, so this was multiplying by nthreads twice. I'm about to open a PR
Given that profiling samples all threads (except windows that only profiles the main thread) it seems to make sense to allocate nsamples per thread (except windows).